Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usb: Add high-level USB device support packages #558

Merged
merged 1 commit into from Apr 30, 2024

Conversation

projectgus
Copy link
Contributor

@projectgus projectgus commented Oct 26, 2022

High-level support packages for creating USB device functionality in Python code.

These modules rely on the machine.USBDevice() object newly merged for MicroPython 1.23 (micropython/micropython#9497).

IMPORTANT: The latest version of this PR requires a low-level change in MicroPython's USBDevice that was merged in micropython/micropython#14253 - nightly builds from before 18 April 2024 do not have this change, and will fail in unexpected ways!!

How to test/use this PR

This PR requires a nightly build of Micropython 1.23-preview or later, on a port with machine.USBDevice support (currently samd or rp2).

Can either edit manifest.py to freeze usb-device packages into the firmware, or use the following command to install packages directly from this PR:

$ mpremote connect /dev/ttyUSB0 mip install --index https://projectgus.github.io/micropython-lib/mip/feature/usbd_python usb-device-NAME

See the README in this PR for a list of package names.

Example Code

See the usb/examples/device subdirectory for some simple example programs.

These aren't installed with the package but can be run as (for example) mpremote run mouse_example.py.

(Note: because any USB serial connection will close on reenumeration, printed output from this point will only be visible if outputting stdout to traditional UART, semihosting, etc.)

Debugging Tips

See the micropython PR micropython/micropython#9497

TODOs

This PR is nearly complete, aside from code review comments I have a few things left to finish:

  • Complete README with some class reference information.
  • Re-test on Windows.
  • Test all examples on macOS (help wanted as I don't have a Mac on hand).

Future work (not this PR)

  • Add an async USB interface helper. As USB is host-initiated and mostly uses callbacks, there isn't a whole lot more work needed here - however some classes would benefit from async variants.
  • See if we can provide a host helper program for generating HID Report Descriptors (or link to one that already exists). I tried implementing this in MicroPython so it builds the Report Descriptor on the device at runtime, but this takes up too much space.

This work was funded through GitHub Sponsors.

@paulhamsh

This comment was marked as outdated.

@jimmo

This comment was marked as outdated.

@paulhamsh

This comment was marked as outdated.

@projectgus

This comment was marked as outdated.

@projectgus

This comment was marked as outdated.

@paulhamsh

This comment was marked as outdated.

@projectgus

This comment was marked as outdated.

@paulhamsh

This comment was marked as outdated.

@paulhamsh

This comment was marked as outdated.

@paulhamsh
Copy link

Hi - my Midi code works fine now - my Python obviously isn't up to it, too many 'self's missing.
Thanks for your help!
https://github.com/paulhamsh/Micropython-Midi-Device

@projectgus
Copy link
Contributor Author

projectgus commented Nov 8, 2022

@paulhamsh That's great! I hope it's useful. Debugging is definitely the most fiddly part of this, especially if you don't have the REPL moved off of the default USB-CDC port! I scratched my head a number of times before I got a robust way to always see errors, and then things got smoother! I'll try to include some debugging tips when I write up the docs for this.

I see you've MIT licensed your midi.py file (thanks!) Would you happy for me to use that as a basis for inclusion in this PR?

@paulhamsh
Copy link

@paulhamsh That's great! I hope it's useful. Debugging is definitely the most fiddly part of this, especially if you don't have the REPL moved off of the default USB-CDC port! I scratched my head a number of times before I got a robust way to always see errors, and then things got smoother! I'll try to include some debugging tips when I write up the docs for this.

I see you've MIT licensed your midi.py file (thanks!) Would you happy for me to use that as a basis for inclusi

@paulhamsh That's great! I hope it's useful. Debugging is definitely the most fiddly part of this, especially if you don't have the REPL moved off of the default USB-CDC port! I scratched my head a number of times before I got a robust way to always see errors, and then things got smoother! I'll try to include some debugging tips when I write up the docs for this.

I see you've MIT licensed your midi.py file (thanks!) Would you happy for me to use that as a basis for inclusion in this PR?

Yes of course! Happy for you to share it!!

@hoihu
Copy link
Sponsor

hoihu commented Apr 24, 2023

I scratched my head a number of times before I got a robust way to always see errors, and then things got smoother! I'll try to include some debugging tips when I write up the docs for this.

@projectgus t I'm struggling with similar issues I guess ;) I wanted the REPL log to UART0 or 1, so I tried os.dupterm(machine.UART(0)) and os.dupterm(machine.UART(1)) but without success. The first one just freezes the REPL on USB, the second one succeeds but I did not see any output on the serial console on UART1.

I also tried enabling MICROPY_HW_ENABLE_UART_REPL and recompiled, with same outcome.

Can you share a few details on your debug setup? Did I overlook something obvious here? Thanks.

@hoihu
Copy link
Sponsor

hoihu commented Apr 24, 2023

I wanted the REPL log to UART0 or 1, so I tried os.dupterm(machine.UART(0)) and os.dupterm(machine.UART(1)) but without success. The first one just freezes the REPL on USB, the second one succeeds but I did not see any output on the serial console on UART1.

never mind, it was too late during that evening I guess, I had the wrong pins connected. REPL works now via UART. Sorry for the noise

@barafael
Copy link

I am trying to get serial data off of my pico and stumbled upon these changes and they look fantastical - I want to create a second usb serial device, which leaves the existing serial connection intact. Is this possible with these changes, even with reenumeration?

@projectgus
Copy link
Contributor Author

@barafael Sorry I didn't reply to you earlier about this. I've had almost no time to work on MP lately, but I've dedicated time from July so the momentum should pick up on this PR quite soon.

I know @hoihu started working on CDC serial support on their fork, there is a branch here with a commit: https://github.com/hoihu/micropython-lib/commits/feat-cdc-python - I haven't tested it, it looks like it would enumerate but there's still a bunch of work before it can be used the same as the default USB serial port.

So, unfortunately I don't know of an easy way to get >1 USB-CDC serial port for now - either with or without these changes. Best I can offer is: watch this space!

@hoihu
Copy link
Sponsor

hoihu commented Jun 8, 2023

but there's still a bunch of work before it can be used the same as the default USB serial port.

Had some time left today and I pushed an update with a basic example on how to use it.
https://github.com/hoihu/micropython-lib/blob/feat-cdc-python/micropython/usbd/cdc_example.py

But as @projectgus said, it's currently still far away from anything. Especially the read() function should be using a ringbuffer or similar. At least it's possible to do some basic rx/tx over that interface now..

@andrewleech
Copy link
Sponsor Contributor

@hoihu does this sound like it's going to be needed? micropython/micropython#9458

@turmoni
Copy link

turmoni commented Jun 28, 2023

Just for the benefit of anyone else looking at this and wanting to borrow code; I've got a HID keypad and basic USB mass storage in my fork. I've created a draft PR to make the changes easy to find at projectgus#1 .

@robin7331

This comment was marked as outdated.

@projectgus

This comment was marked as outdated.

@projectgus projectgus force-pushed the feature/usbd_python branch 2 times, most recently from 53f9558 to 581a662 Compare July 10, 2023 08:06
@projectgus
Copy link
Contributor Author

projectgus commented Jul 10, 2023

Rebased this PR and the linked micropython PR, and incorporated some fixes for issues noticed by @turmoni and @hoihu . EP_IN_FLAG is now correctly defined as the right constant.

@turmoni I've implemented a possible alternative fix in this branch for the _always_cb workaround in your PR. There was a race where the callback function wasn't added before submit_transfer was called, so a very rapid xfer completion could fire before there was a callback set. This might not be enough to solve the recursion depth crash you were seeing if the host sends back a lot of rapid responses though, I haven't tested your MSC class with it. It turns out that there are some cases where the current implementation requires usbd_task() to recurse. Doing every device callback via micropython.schedule() might turn out to be the path of least resistance.

(EDIT: Sorry @turmoni I just recalled you'd documented this race in detail over on micropython/micropython#9497 (comment) and I'd missed it as I was busy with other things. There might be more to this, but also there might not be!)

@projectgus
Copy link
Contributor Author

@turmoni Regarding the issues you mentioned:

The inability to STALL endpoints

Now implemented as USBInterface.set_ep_stall() & USBInterface.get_ep_stall(). Have tested these work "on the wire" but not in a context where they matter for protocol correctness (like MSC).

Control transfer callbacks not firing for anything other than the first interface that's added

Do you remember which transfers you weren't seeing interface callbacks for?

Not really seeing a way to handle input on the control endpoint

This was possible but fiddly. I've adapted the keypad example you wrote to use control transfers for SET_REPORT and added it here.

Am currently trying to decide if it's worth building a more intuitive interface for control transfers with data, or to leave it like this.

@jzhvymetal
Copy link

jzhvymetal commented Apr 8, 2024

I fixed the issues above by creating a shared variable import so that both 'boot.py' and 'code.py' can access it.

Now, the problem I'm encountering is that the mouse performance is very slow and lags. I am working on a project that involves creating a Barrier Client (https://github.com/jzhvymetal/PY_Barrier) which receives mouse and keyboard commands via Barrier Server over WiFi and then relays them to HID. I implemented the same code on CircuitPython, and the response from the mouse has no lag. Additionally, I implemented it in both MP and CP by using a serial CH9329, which also showed no lag. Is there anything I'm missing to improve the MP HID performance?

All of my code is available on the Git repository below. Simply copy all the .py files to the 'lib' directory and place boot.py' in the root.

https://github.com/jzhvymetal/PY_Barrier

@hoihu
Copy link
Sponsor

hoihu commented Apr 8, 2024

just as a follow up to my midi tests. I used MIDI Monitor to debug the midi notes and the notes come in reliably, with the control value increasing, as expected:

image

It let that run for several hours without issues.

@projectgus
Copy link
Contributor Author

Now, the problem I'm encountering is that the mouse performance is very slow and lags.

Neat project, @jzhvymetal! Where you call super().send_report(self.report), try adding timeout_ms=0 to the arguments.

(If an existing report is pending, send_report will block for up to the timeout - default 100ms - before returning. Because you're queueing the same buffer object each time, you can return immediately - the contents of the buffer are what is sent, so if there's one pending then it should be updated by the time the report is actually sent to the host.)

This part of the base class might need to be changed (at minimum, possibly change the default to 0ms.)

If this isn't it, let me know and I'll debug some more.

@projectgus
Copy link
Contributor Author

projectgus commented Apr 8, 2024

But I also observed that I did not get any print statements after the example starts.
...
I digged a little deeper in the non-print-statements and it looks that def _rx_cb(self, ep, res, num_bytes): is not called (in midi.py).

@hoihu I think this might be a poorly commented example on my part. The example does two things - it sends MIDI notes in a loop for as long as the code keeps running. This part doesn't print anything.

At the same time, if the host sends any MIDI data to the device then it should print something when it receives that MIDI data.

Each direction (IN/OUT) is usually configured separately in MIDI settings (as most devices only transfer MIDI one way). So if the device is configured in Garage Band as only a MIDI input, the current example doesn't print anything. There is also sometimes an option on the host named "MIDI Thru" (or similar), in this mode the host will echo the received "IN" midi messages back "OUT" to that device.

I'll change the example so it prints when operating in both directions.

any recommendation on simpler tools than garage band to interact with midi devices?

I'm not a MIDI expert either, so my go-to for testing has been the Virtual MIDI Piano Keyboard. When configured with an external device as MIDI Input, the keys are highlighted when notes are received. When configured with an external device as MIDI Output, pressing keys on the (computer) keyboard sends note messages. (Similarly for control channel data there are virtual knobs that work both ways.)

@projectgus
Copy link
Contributor Author

Hmm OK, it looks like there is something else going on with MIDI Out. In some of my runs it works fine in both directions, but sometimes the host sends a few MIDI OUT packets and then stops (MIDI IN keeps flowing). I don't know why, the USB traffic shows the transfers are successful.

This is on Linux with ALSA but I expect whatever the bug is might be the same on all OSes. Still debugging...

@jzhvymetal
Copy link

jzhvymetal commented Apr 9, 2024

Neat project, @jzhvymetal!

Thanks...needed to learn Python what better way than have a project

Where you call super().send_report(self.report), try adding timeout_ms=0 to the arguments.

Perfect, this fixed the problem... I should have delved deeper down the rabbit hole. Now it seems obvious, with the argument named timeout_ms. I also lost 2 hours because the Git version of my code for KM_MPHID_ACTORS.py was not the latest. I must have forgotten that I saved it down at the board level and didn't make a local copy. 🤦 I also forgot that another board was running with the same device name, which rejects the second device connecting. 🤦🤦

@projectgus
Copy link
Contributor Author

projectgus commented Apr 9, 2024

This is on Linux with ALSA but I expect whatever the bug is might be the same on all OSes. Still debugging...

After checking my computer's MIDI setup again I can't reproduce this any more, both ways are working.

Given the USB captures don't show any failed transfers or invalid MIDI data, I'm going to put this down to a problem with my computer's MIDI configuration. Have pushed a midi_example.py that prints more output, please let me know if it's still not working as expected.

@jzhvymetal
Copy link

Where you call super().send_report(self.report), try adding timeout_ms=0 to the arguments.

Follow-up question: I updated my code to include a custom keyboard device. I noticed that if I set the value to 0, it would sometimes keep the keys down even though the report was sent again without it. I changed the value to 3ms, and it seems to fix the problem. What is the proper usage, and when should you include a delay?

Reviewing the code from CircuitPython custom descriptors, they include a report ID, for example, 0x85, 0x0B, # Report ID [11 is SET at RUNTIME], and you pass the ID to the creation of the HID device. With the MP implementation, I had to comment out the Report ID. Is there any way to keep the Report ID for consistency?

Once again, thanks for the implementation. Without it, I would not have been able to code my project on MP. I know the main focus is on rp2 and SAMD boards, but if you managed to also get ESP32 working. I would love to test it. In any case, great job so far.

@hoihu
Copy link
Sponsor

hoihu commented Apr 12, 2024

please let me know if it's still not working as expected.

yes it's clear now and the example works. Thanks for the updates.

I also tested the mouse and keyboard_hid examples. They work too on macOS. So looking really good here 👍

@projectgus
Copy link
Contributor Author

Hi @jzhvymetal,

Follow-up question: I updated my code to include a custom keyboard device. I noticed that if I set the value to 0, it would sometimes keep the keys down even though the report was sent again without it. I changed the value to 3ms, and it seems to fix the problem. What is the proper usage, and when should you include a delay?

Yes, in retrospect setting timeout_ms=0 was papering over the root cause of the lags you were seeing: when a timeout was set, send_report() was sleeping for 50ms between tries. This was way too long and probably causing the visible stalls/jerkiness. If you pull the latest version of this PR then you can hopefully go back to the default 100ms without any stalls, or any lost reports.

(There is still potentially a race condition that could miss an event when the caller reuses the same bytearray object as the buffer every time, an interim report might not be sent to the host because its data has been replaced by a later report. I've just updated the PR so mouse.py and keyboard.py demonstrate two different ways to avoid this - mouse.py checks busy() before updating the buffer, and keyboard.py uses double buffering. However this also might not be a problem you care about, it shouldn't cause stuck keys or buttons.)

Reviewing the code from CircuitPython custom descriptors, they include a report ID, for example, 0x85, 0x0B, # Report ID [11 is SET at RUNTIME], and you pass the ID to the creation of the HID device. With the MP implementation, I had to comment out the Report ID. Is there any way to keep the Report ID for consistency?

Yes, report IDs are optional in USB HID but they should work. Include them in the HID descriptors, same as CircuitPython does, and then pass the report ID as as the first byte in the data argument of send_report(). (CircuitPython uses the TinyUSB driver, and it passes the report ID as a separate argument but then copies it as the first byte of the buffer that is sent "on the wire".)

Once again, thanks for the implementation. Without it, I would not have been able to code my project on MP. I know the main focus is on rp2 and SAMD boards, but if you managed to also get ESP32 working. I would love to test it. In any case, great job so far.

Thanks! Supporting ESP32-S series is on the radar. There is some low level work that needs to be done first, because ESP-IDF runs TinyUSB in its own RTOS task. This makes it harder to hook the TinyUSB callbacks into MicroPython, but it will be possible to make it work.

@projectgus
Copy link
Contributor Author

Have performed another rebase & squash including a breaking change for anyone who has implemented their own class driver: usb.device.impl module is now named usb.device.core.

@projectgus projectgus marked this pull request as ready for review April 16, 2024 05:15
@rafaellago
Copy link

Hello, not sure I'm missing something, I was trying the KeyboardExample provided, and when it gets to usb.device.get().init(k, builtin_driver=True) my rpi pico just reboots. I'm using MP v1.23.0-preview.332.g49ce7a607, on win10 and thonny (alhtough I tested connecting directly to it via puTTY and had the same behaviour sending the code via the REPL).

Sample sending the lines through the REPL

MicroPython v1.23.0-preview.332.g49ce7a607 on 2024-04-22; Raspberry Pi Pico with RP2040

Type "help()" for more information.

>>> import usb.device
>>> from usb.device.keyboard import KeyboardInterface
>>> kbd = KeyboardInterface()
>>> usb.device.get().init(kbd, builtin_driver=True)
PROBLEM IN THONNY'S BACK-END: Exception while handling 'execute_source' (ConnectionError: EOF).
See Thonny's backend.log for more info.
You may need to press "Stop/Restart" or hard-reset your MicroPython device and try again.


Process ended with exit code 1.
────────────────────

Connection lost -- EOF

Use Stop/Restart to reconnect.

Process ended with exit code 1.

Also backend.log from thonny if that helps somehow.

backend.log

Thanks for any help, and sorry if I'm not being smart enough.

@projectgus
Copy link
Contributor Author

projectgus commented Apr 23, 2024

Hello, not sure I'm missing something, I was trying the KeyboardExample provided, and when it gets to usb.device.get().init(k, builtin_driver=True) my rpi pico just reboots

Hi @rafaellago,

Thanks for the comprehensive report, and you are certainly being smart enough.

The big limitation with the dynamic USB support is that reconfiguring the USB interface requires disconnecting any existing USB device (i.e. the USB serial port holding the REPL in most cases), as we have to simulate an unplug-then-replug of the whole USB device so the host sees any new interfaces. This also has to happen if MicroPython soft resets while a dynamic USB device is currently active.

If the whole program has already been sent to MicroPython then it may actually be running after the serial port goes away and comes back, and you could reconnect with a program like PuTTY and see the output from the running program.

How well any of this works with Thonny is an open question, I haven't looked closely at Thonny's behaviour - it may be that you can also reconnect in Thonny after getting this error and your program will be running with the dynamic USB active, but it also could be that Thonny sends a soft reset each time it reconnects and this is causing the dynamic USB device to immediately disappear again (along with the USB COM port).

I'm planning some work to improve mpremote's support for the USB serial port temporarily going away like this, and I think something similar could be added to Thonny. However, as I said I haven't looked into Thonny specifically and any feature would obviously be up to the Thonny maintainers.

@rafaellago
Copy link

Hello @projectgus

As you said, maybe Thonny is restarting the pico on the connection loss, testing through putty and restarting the session actually brought me back to the repl and I managed to send keystrokes to my PC.

If I manage to make a multimedia keyboard example (just volume up/down/mute) can I send a PR to your repo? Or is it better to send it directly to the micropython one?

Thank you so much for your explanation.

@projectgus
Copy link
Contributor Author

If I manage to make a multimedia keyboard example (just volume up/down/mute) can I send a PR to your repo? Or is it better to send it directly to the micropython one?

This PR is already pretty huge so I think the best would be to wait until this PR is merged and then submit a new PR against this repo that just adds the example.

If you have any questions as you work on the keyboard program then feel free to ask. Although if they're not directly related to this PR then it might be best to start a Discussion and then make a comment here that links to it (otherwise things tend to pile up a bit.)

@f4grx
Copy link

f4grx commented Apr 26, 2024

Hi,

You've done a tremendous work on this project, thank you for all this.

Whats missing to get it merged?

@rafaellago
Copy link

wait until this PR is merged

Alright, fair enough. I discovered it will take me a while to get my head around hid descriptors, so until then, it will probably be already merged. hahaha thank you so much

Whats missing to get it merged?

@f4grx testing on mac os, I think

@projectgus
Copy link
Contributor Author

Whats missing to get it merged?

@f4grx testing on mac os, I think

Oops, I neglected to update the TODO in the first post until just now, but @hoihu has tested the examples on macos without issue 🙏

The README is still a little bare bones, but the main thing that's still pending is probably code review from some of the other maintainers.

micropython/usb/README.md Outdated Show resolved Hide resolved
micropython/usb/README.md Outdated Show resolved Hide resolved
micropython/usb/README.md Outdated Show resolved Hide resolved
micropython/usb/README.md Outdated Show resolved Hide resolved
micropython/usb/README.md Outdated Show resolved Hide resolved
micropython/usb/examples/device/cdc_repl_example.py Outdated Show resolved Hide resolved
micropython/usb/usb-device/tests/test_core_buffer.py Outdated Show resolved Hide resolved
micropython/usb/usb-device/tests/test_core_buffer.py Outdated Show resolved Hide resolved
micropython/usb/usb-device/usb/device/core.py Outdated Show resolved Hide resolved
These packages build on top of machine.USBDevice() to provide high level
and flexible support for implementing USB devices in Python code.

Additional credits, as per included copyright notices:

- CDC support based on initial implementation by @hoihu with fixes by
  @linted.

- MIDI support based on initial implementation by @paulhamsh.

- HID keypad example based on work by @turmoni.

- Everyone who tested and provided feedback on early versions of these
  packages.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@dpgeorge dpgeorge merged commit 583bc0d into micropython:master Apr 30, 2024
3 checks passed
@dpgeorge
Copy link
Member

Now merged!! Thanks to everyone who contributed here, with testing, feedback and coding.

@f4grx
Copy link

f4grx commented Apr 30, 2024

Thank you!

@projectgus
Copy link
Contributor Author

Folks, now this PR is merged some good places to continue discussion are:

... and of course if you find a bug then please open a new Issue on this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet